Add Lua module to serve SSL Certificates dynamically #2965

hnrytrn · 2018-08-21T17:51:30Z

What this PR does / why we need it:

This PR ties in #2889 and #2923 to add dynamic certificate serving functionality. When enable-dynamic-certificates is on, SSL Certificates won't be written on disk, and will be served dynamically from the shared Lua dictionary using OpenResty's SSL library (https://github.com/openresty/lua-resty-core/blob/master/lib/ngx/ssl.md), which will avoid reloading NGINX. This PR also adds in E2E tests for the dynamic certificates functionality.

kfox1111 · 2018-08-21T18:05:45Z

Interesting. Could this mechanism also be used to support having a CA specified in the ingress to be used when communicating with the backend service?

ElvinEfendi · 2018-08-22T16:15:37Z

test/e2e/lua/dynamic_certificates.go

+	BeforeEach(func() {
+		err := enableDynamicCertificates(f.IngressController.Namespace, f.KubeClientSet)
+		Expect(err).NotTo(HaveOccurred())
+


let's also assert here that dynamic certificate feature is indeed enabled:

err = f.WaitForNginxConfiguration( func(cfg string) bool { return strings.Contains(cfg, "ssl_certificate_by_lua_block") && strings.Contains(cfg, "certificate.call()") }) Expect(err).NotTo(HaveOccurred())

ElvinEfendi · 2018-08-22T16:16:42Z

test/e2e/lua/dynamic_certificates.go

+	return framework.UpdateDeployment(kubeClientSet, namespace, "nginx-ingress-controller", 1,
+		func(deployment *appsv1beta1.Deployment) error {
+			args := deployment.Spec.Template.Spec.Containers[0].Args
+			args = append(args, "--enable-dynamic-configuration")


no need for this, it's enabled by default

ElvinEfendi · 2018-08-22T18:31:29Z

rootfs/etc/nginx/lua/certificate.lua

+
+  local pem_cert_key = configuration.get_pem_cert_key(hostname)
+  if not pem_cert_key then
+    ngx.log(ngx.ERR, "Certificate not found for the given hostname: " .. hostname)


hostname needs to be casted to string, otherwise when it is nil it will error

ElvinEfendi · 2018-08-22T18:33:41Z

test/e2e/lua/dynamic_certificates.go

+				Set("Host", ingress.Spec.TLS[0].Hosts[0]).
+				TLSClientConfig(&tls.Config{
+					InsecureSkipVerify: true,
+				}).


since you are providing a custom config you have to explicitly set the ServerName to ingress.Spec.TLS[0].Hosts[0]

ElvinEfendi · 2018-08-22T18:34:05Z

test/e2e/lua/dynamic_certificates.go

+				return err
+			}
+
+			return nil


you can remove this all if stuff and do return err

ElvinEfendi · 2018-08-22T19:27:12Z

test/e2e/lua/dynamic_certificates.go

+					Hosts:      []string{"foo.com"},
+					SecretName: "foo.com",
+				},
+			}


this is redundant, when creating Ingress the framework already does this

ElvinEfendi · 2018-08-23T01:11:04Z

As discussed IRL this as is won't work since we don't consistently fallback to default certificate when secret is missing and end up configuring HTTP only server even though TLS is included in the Ingress manifest. I created #2972 to fix this. That PR also has more info on why we need that change.

codecov-io · 2018-08-24T02:41:46Z

Codecov Report

Merging #2965 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2965      +/-   ##
==========================================
- Coverage   47.54%   47.52%   -0.02%     
==========================================
  Files          77       77              
  Lines        5639     5639              
==========================================
- Hits         2681     2680       -1     
- Misses       2605     2607       +2     
+ Partials      353      352       -1

Impacted Files	Coverage Δ
internal/ingress/controller/nginx.go	`16.04% <100%> (ø)`	⬆️
internal/watch/file_watcher.go	`80.76% <0%> (-3.85%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 332b3ad...cbf041f. Read the comment docs.

ElvinEfendi · 2018-08-24T03:26:14Z

There are quite a bit of repetition in e2e test implementation but that can be iterated later

ElvinEfendi · 2018-08-24T03:26:27Z

/hold cancel
/lgtm
/approve

ElvinEfendi · 2018-08-24T03:26:50Z

/approve

k8s-ci-robot · 2018-08-24T03:26:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ElvinEfendi, hnrytrn

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ElvinEfendi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ElvinEfendi · 2018-08-25T03:59:54Z

Could this mechanism also be used to support having a CA specified in the ingress to be used when communicating with the backend service?

@kfox1111 are you asking if we can dynamically controler proxy_ssl_trusted_certificate? If so, to my knowledge lua-ngx-module does not provide any API for that. Also why would you wanna do that? Sorry it isn't clear to me what you mean.

discostur · 2018-09-10T13:50:52Z

@hnrytrn according to the documentation (https://github.com/Shopify/ingress/blob/master/docs/user-guide/cli-arguments.md) this feature is capable of storing roughly 5000 certificates:

Assuming the certificate is generated with a 2048 bit RSA key/cert pair, this feature can store roughly 5000 certificates.

Can you explain to me how you came to the max of 5000 certificates? What is the limiting factor?

ElvinEfendi · 2018-09-10T14:22:12Z

hey @discostur , we use Lua shared dictionary (with fixed size) to store the certificates in memory. You can find the configuration at

ingress-nginx/internal/ingress/controller/template/template.go

Line 195 in 0a9db37

"lua_shared_dict certificate_data 16M",

It's currently configured to store up to 16M data, so the limit comes from there. About the calculation please check Shopify#56 (comment)

Do you need to be able to handle more certificates?

discostur · 2018-09-10T14:46:51Z

@ElvinEfendi thanks for the explanation. Yes indeed we ware actually trying to put up to 100.000 vhosts / certificates into nginx ingress. If it is only dependent from the storage setting, would it be an idea to make this setting configurable?

ElvinEfendi · 2018-09-10T15:51:16Z

@discostur it would definitely make sense to make it configurable. We can introduce a new configmap key for that. I'd be happy to review if you make a PR.

kfox1111 · 2019-07-09T22:35:38Z

@ElvinEfendi yes. securing the communications channel between ingress nginx to the service via the ca used to sign the services certificate. Each service could have its own ca, so should be able to be specified individually in the ingress. something like cert-manager could be used to provision all the service certs easily. Just no way currently to plumb it all together.

ElvinEfendi · 2019-07-09T22:43:45Z

Each service could have its own ca

Why not have intermediate CA and issue the certificates for the services using the intermediate CA? This way you can install only that CA in the ingress-nginx.

--

But yeah to answer your question, no dynamically configuring proxy_ssl_trusted_certificate is not possible.

kfox1111 · 2019-07-09T23:25:01Z

That could be done. but then puts somewhat of the onus on managing the ca for the entire cluster, which is quite hard. If you could specify what ca to use per service, the service could make up one on the fly and fairly transparently, but it still would be secure from other folks on the same cluster.

Just looking for ways of making security easier so folks do it. Ideally fully automatable, so they don't have to think about it at all. :)

kfox1111 · 2019-07-09T23:28:15Z

the nginx.ingress.kubernetes.io/secure-verify-ca-secret: "my-ca-secret" code looks like it just about would do what we want. Not sure if it works anymore. Its not documented and there was a closed pr against it.

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 21, 2018

aledbf assigned ElvinEfendi Aug 21, 2018

hnrytrn mentioned this pull request Aug 21, 2018

Add ssl certificate lua directive Shopify/ingress#60

Closed

hnrytrn force-pushed the dynamic-certificates-nginx branch 4 times, most recently from d00509c to 78d132f Compare August 22, 2018 15:02

ElvinEfendi reviewed Aug 22, 2018

View reviewed changes

hnrytrn force-pushed the dynamic-certificates-nginx branch 2 times, most recently from 07409ba to 3b92465 Compare August 22, 2018 18:48

ElvinEfendi reviewed Aug 22, 2018

View reviewed changes

ElvinEfendi mentioned this pull request Aug 23, 2018

consistently fallback to default certificate when TLS is configured #2972

Merged

hnrytrn force-pushed the dynamic-certificates-nginx branch 3 times, most recently from 18b6839 to 153382f Compare August 23, 2018 18:15

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 23, 2018

Add Lua module to serve SSL Certificates dynamically

cbf041f

hnrytrn force-pushed the dynamic-certificates-nginx branch from 6da4110 to cbf041f Compare August 24, 2018 02:16

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 24, 2018

k8s-ci-robot merged commit b0b575d into kubernetes:master Aug 24, 2018

ElvinEfendi deleted the dynamic-certificates-nginx branch August 24, 2018 03:28

aledbf mentioned this pull request Mar 1, 2019

Where are tls.crt and tls.key available? #3832

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Lua module to serve SSL Certificates dynamically #2965

Add Lua module to serve SSL Certificates dynamically #2965

hnrytrn commented Aug 21, 2018

kfox1111 commented Aug 21, 2018

ElvinEfendi Aug 22, 2018

ElvinEfendi Aug 22, 2018

ElvinEfendi Aug 22, 2018

ElvinEfendi Aug 22, 2018

ElvinEfendi Aug 22, 2018

ElvinEfendi Aug 22, 2018

ElvinEfendi commented Aug 23, 2018

codecov-io commented Aug 24, 2018

ElvinEfendi commented Aug 24, 2018

ElvinEfendi commented Aug 24, 2018

ElvinEfendi commented Aug 24, 2018

k8s-ci-robot commented Aug 24, 2018

ElvinEfendi commented Aug 25, 2018

discostur commented Sep 10, 2018

ElvinEfendi commented Sep 10, 2018

discostur commented Sep 10, 2018

ElvinEfendi commented Sep 10, 2018

kfox1111 commented Jul 9, 2019

ElvinEfendi commented Jul 9, 2019

kfox1111 commented Jul 9, 2019

kfox1111 commented Jul 9, 2019

Add Lua module to serve SSL Certificates dynamically #2965

Add Lua module to serve SSL Certificates dynamically #2965

Conversation

hnrytrn commented Aug 21, 2018

kfox1111 commented Aug 21, 2018

ElvinEfendi Aug 22, 2018

Choose a reason for hiding this comment

ElvinEfendi Aug 22, 2018

Choose a reason for hiding this comment

ElvinEfendi Aug 22, 2018

Choose a reason for hiding this comment

ElvinEfendi Aug 22, 2018

Choose a reason for hiding this comment

ElvinEfendi Aug 22, 2018

Choose a reason for hiding this comment

ElvinEfendi Aug 22, 2018

Choose a reason for hiding this comment

ElvinEfendi commented Aug 23, 2018

codecov-io commented Aug 24, 2018

Codecov Report

ElvinEfendi commented Aug 24, 2018

ElvinEfendi commented Aug 24, 2018

ElvinEfendi commented Aug 24, 2018

k8s-ci-robot commented Aug 24, 2018

ElvinEfendi commented Aug 25, 2018

discostur commented Sep 10, 2018

ElvinEfendi commented Sep 10, 2018

discostur commented Sep 10, 2018

ElvinEfendi commented Sep 10, 2018

kfox1111 commented Jul 9, 2019

ElvinEfendi commented Jul 9, 2019

kfox1111 commented Jul 9, 2019

kfox1111 commented Jul 9, 2019